feat: fix IBC #101
feat: fix IBC #101
Conversation
WalkthroughThis update introduces several new CometBFT-adapter components for Rollkit integration, modifies block and commit hash computation, and updates the control flow for transaction execution and block finalization. It also adds new tests for CometBFT light client compatibility, updates dependency versions, and exposes additional hashing and payload provider functionality throughout the node and RPC environment. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Node
participant Adapter
participant RollkitStore
participant ABCIApp
Client->>Node: Submit block with txs, metadata
Node->>Adapter: ExecuteTxs(txs, blockHeight, timestamp, prevStateRoot, metadata)
Adapter->>RollkitStore: Get previous block commit (if not genesis)
Adapter->>Adapter: cometCommitToABCICommitInfo(prevCommit)
Adapter->>ABCIApp: ProcessProposal/FinalizeBlock (with commit info, header hash)
Adapter->>Adapter: Update state (app hash, height)
Adapter->>Node: Return block result
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…abci into feat/fix-ibc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
pkg/rollkit_adapter/header_hasher.go (2)
9-18: LGTM! Clean implementation of CometBFT header hasher.The function correctly:
- Converts Rollkit headers to CometBFT ABCI headers
- Handles conversion errors appropriately
- Returns the proper hash type
Consider adding error context for better debugging:
func CreateCometBFTHeaderHasher() types.HeaderHasher { return func(header *types.Header) (types.Hash, error) { abciHeader, err := common.ToABCIHeader(header) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to convert header to ABCI format: %w", err) } return types.Hash(abciHeader.Hash()), nil } }
3-7: Consider adding fmt import for enhanced error messaging.If you implement the error context suggestion above, add the fmt import:
import ( + "fmt" + "github.com/rollkit/rollkit/types" "github.com/rollkit/go-execution-abci/pkg/common" )pkg/rpc/core/blocks_test.go (1)
245-249: Consider adding documentation for the 32-byte requirement.The function correctly pads/truncates input to 32 bytes, but it would be helpful to document why 32 bytes is required (likely for hash compatibility).
Add a more descriptive comment:
-// Renamed and modified helper function to return []byte of 32 length +// BytesToSliceHash converts arbitrary byte slices into fixed 32-byte slices +// by copying input bytes and padding with zeros if needed. This ensures +// compatibility with CometBFT hash requirements which expect 32-byte hashes. func BytesToSliceHash(b []byte) []byte {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
go.mod(2 hunks)pkg/adapter/adapter.go(8 hunks)pkg/adapter/adapter_test.go(1 hunks)pkg/common/convert.go(3 hunks)pkg/rollkit_adapter/commit_hasher.go(1 hunks)pkg/rollkit_adapter/header_hasher.go(1 hunks)pkg/rollkit_adapter/signer.go(1 hunks)pkg/rollkit_adapter/validator_hasher.go(1 hunks)pkg/rpc/core/blocks.go(2 hunks)pkg/rpc/core/blocks_test.go(2 hunks)pkg/rpc/core/env.go(2 hunks)pkg/rpc/utils.go(2 hunks)server/start.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
pkg/rpc/utils.go (1)
pkg/rpc/core/blocks.go (1)
Commit(166-194)
pkg/rollkit_adapter/commit_hasher.go (1)
pkg/rpc/utils.go (1)
GetABCICommit(128-147)
server/start.go (4)
pkg/rollkit_adapter/validator_hasher.go (1)
CreateCometBFTValidatorHasher(18-64)pkg/rollkit_adapter/signer.go (1)
CreateCometBFTPayloadProvider(11-34)pkg/rollkit_adapter/header_hasher.go (1)
CreateCometBFTHeaderHasher(9-18)pkg/rollkit_adapter/commit_hasher.go (1)
CreateCometBFTCommitHasher(9-16)
pkg/common/convert.go (1)
pkg/rpc/core/blocks.go (2)
Block(92-137)Commit(166-194)
🪛 GitHub Check: lint / golangci-lint
pkg/rollkit_adapter/signer.go
[failure] 5-5:
File is not properly formatted (gci)
pkg/rollkit_adapter/validator_hasher.go
[failure] 12-12:
File is not properly formatted (gci)
🪛 GitHub Actions: CI and Release
pkg/rollkit_adapter/signer.go
[error] 5-5: File is not properly formatted (gci)
🔇 Additional comments (17)
go.mod (2)
30-31: LGTM! Dependency updates support CometBFT integration.The updated versions for rollkit dependencies align with the PR objectives to introduce CometBFT-adapter components.
358-359: Verify the commented-out replace directives are intentional.These commented replace directives suggest local development setup. Ensure they're intentionally included in the commit or remove them before merging to production.
pkg/rpc/core/env.go (1)
12-12: LGTM! Properly imports required types.The import addition supports the new HeaderHasher field integration.
pkg/adapter/adapter_test.go (1)
90-90:✅ Verification successful
Verify ExecuteTxs method signature compatibility.
The test has been updated to include an additional
nilparameter, reflecting the new metadata parameter in the ExecuteTxs method signature.
🏁 Script executed:
#!/bin/bash # Description: Verify ExecuteTxs method signature matches the test call # Expected: Find ExecuteTxs method definition with metadata parameter # Search for ExecuteTxs method definition ast-grep --pattern 'func ($$$) ExecuteTxs($$$) ($$$) { $$$ }'Length of output: 12408
ExecuteTxs metadata parameter verified
The
ExecuteTxsmethod inpkg/adapter/adapter.go:280is declared as:func (a *Adapter) ExecuteTxs( ctx context.Context, txs [][]byte, blockHeight uint64, timestamp time.Time, prevStateRoot []byte, metadata map[string]interface{}, ) ([]byte, uint64, error)which matches the extra
nilpassed in the test atpkg/adapter/adapter_test.go:90. No changes required—approving the update.pkg/rpc/utils.go (1)
88-88: LGTM! Necessary visibility change for CometBFT integration.The function is correctly exported to allow usage in the
rollkit_adapterpackage, which is essential for the CometBFT commit hasher implementation.Also applies to: 126-128
pkg/rollkit_adapter/signer.go (1)
11-34: LGTM! Well-implemented CometBFT vote payload provider.The implementation correctly:
- Converts the Rollkit header to ABCI format with proper error handling
- Constructs a valid CometBFT precommit vote with all required fields
- Generates the appropriate sign bytes for the vote
server/start.go (1)
414-418: LGTM! Clean integration of CometBFT adapter components.The changes properly integrate all four CometBFT adapter components:
- Validator hasher (with appropriate logger)
- Payload provider for vote signing
- Header hasher for block hashing
- Commit hasher for commit verification
The components are correctly passed to the node constructor and the header hasher is appropriately exposed in the RPC environment.
Also applies to: 445-449, 469-469
pkg/common/convert.go (3)
8-10: LGTM! Clear separation of version imports.The import alias changes improve code clarity by distinguishing between protobuf version types (
cmprotoversion) and version constants (cmtversion).
19-21: Good change for CometBFT compatibility.Using
cmtversion.BlockProtocolas a constant ensures all blocks use a consistent version, which is important for CometBFT light client verification.
98-98: LGTM! More flexible parameter type.Changing from
rlktypes.Hashto[]bytemakes the function more versatile and aligns well with its usage in the RPC handlers where CometBFT block hashes are passed.pkg/rpc/core/blocks.go (2)
118-122: Good addition of centralized header hashing.Using
env.HeaderHasherensures consistent header hash computation across the system, which is crucial for CometBFT compatibility.
179-192: Excellent approach for CometBFT-compatible commits.Converting to CometBFT block first, then using its hash and metadata for the commit ensures proper light client compatibility. This is the correct approach for IBC integration.
pkg/rollkit_adapter/validator_hasher.go (1)
16-64: Excellent implementation of CometBFT-compatible validator hashing.The function properly handles:
- Ed25519 key validation and size verification
- Address derivation and verification against the proposer address
- Proper error handling with descriptive messages
- Good logging for debugging
This is crucial for IBC compatibility as it ensures validator hashes match CometBFT's expectations.
pkg/rpc/core/blocks_test.go (1)
101-242: Comprehensive test for CometBFT light client compatibility!The test thoroughly validates the multi-block light client verification flow. The implementation correctly simulates the creation and verification of multiple sequential blocks with proper state transitions.
pkg/adapter/adapter.go (3)
250-250: LGTM! Proper chain ID initialization.Setting the chain ID on the state during initialization is correct and necessary for proper chain operation.
307-331: Well-structured commit info handling for block transitions.The implementation correctly handles both genesis (empty commit info) and subsequent blocks (retrieving previous block data). The error handling is appropriate.
609-638: Clean implementation of commit conversion function.The helper function properly handles nil commits and empty signatures, with appropriate defensive programming.
| @@ -30,6 +31,8 @@ | |||
| BlockIndexer indexer.BlockIndexer | |||
| Logger cmtlog.Logger | |||
| Config cmtcfg.RPCConfig | |||
|
|
|||
| HeaderHasher types.HeaderHasher | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify HeaderHasher field initialization.
The new HeaderHasher field is properly declared. Ensure it's properly initialized in the Environment setup code.
🏁 Script executed:
#!/bin/bash
# Description: Verify HeaderHasher field is properly initialized in the codebase
# Expected: Find code that assigns a value to Environment.HeaderHasher
# Search for HeaderHasher field assignments
rg -A 3 -B 3 "HeaderHasher.*=" --type go
# Search for Environment struct initialization patterns
rg -A 10 "Environment\s*\{" --type goLength of output: 7933
Initialize HeaderHasher in all test Environment setups
While HeaderHasher is correctly set in server/start.go, the test suites directly instantiate core.Environment without providing a hasher, leading to a nil value under test. Update each test’s Environment initializer to include a valid HeaderHasher (for example, rollkit_adapter.CreateCometBFTHeaderHasher() or a lightweight mock):
- pkg/rpc/core/consensus_test.go
- pkg/rpc/core/blocks_test.go
- pkg/rpc/core/status_test.go
- pkg/rpc/core/tx_test.go
- pkg/rpc/core/abci_test.go
- pkg/rpc/core/evidence_test.go
This ensures any RPC paths using the hasher won’t panic or behave unexpectedly.
🤖 Prompt for AI Agents
In pkg/rpc/core/env.go at line 35, the HeaderHasher field is declared but not
initialized in test Environment setups. To fix this, update all test files
listed (consensus_test.go, blocks_test.go, status_test.go, tx_test.go,
abci_test.go, evidence_test.go) to initialize the Environment struct with a
valid HeaderHasher instance, such as
rollkit_adapter.CreateCometBFTHeaderHasher() or a suitable mock, ensuring no nil
HeaderHasher during tests.
| func CreateCometBFTCommitHasher() types.CommitHashProvider { | ||
| return func(signature *types.Signature, header *types.Header, proposerAddress []byte) (types.Hash, error) { | ||
| abciCommit := rpc.GetABCICommit(header.Height(), header.Hash(), proposerAddress, header.Time(), *signature) | ||
| abciCommit.Signatures[0].ValidatorAddress = proposerAddress | ||
| abciCommit.Signatures[0].Timestamp = header.Time() | ||
| return types.Hash(abciCommit.Hash()), nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant field assignments.
Lines 12-13 are redundant as GetABCICommit already sets these fields with the same values:
ValidatorAddressis set toproposerAddress(line 141 inGetABCICommit)Timestampis set toheader.Time()(line 142 inGetABCICommit)
Apply this diff to remove the redundant assignments:
func CreateCometBFTCommitHasher() types.CommitHashProvider {
return func(signature *types.Signature, header *types.Header, proposerAddress []byte) (types.Hash, error) {
abciCommit := rpc.GetABCICommit(header.Height(), header.Hash(), proposerAddress, header.Time(), *signature)
- abciCommit.Signatures[0].ValidatorAddress = proposerAddress
- abciCommit.Signatures[0].Timestamp = header.Time()
return types.Hash(abciCommit.Hash()), nil
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CreateCometBFTCommitHasher() types.CommitHashProvider { | |
| return func(signature *types.Signature, header *types.Header, proposerAddress []byte) (types.Hash, error) { | |
| abciCommit := rpc.GetABCICommit(header.Height(), header.Hash(), proposerAddress, header.Time(), *signature) | |
| abciCommit.Signatures[0].ValidatorAddress = proposerAddress | |
| abciCommit.Signatures[0].Timestamp = header.Time() | |
| return types.Hash(abciCommit.Hash()), nil | |
| } | |
| } | |
| func CreateCometBFTCommitHasher() types.CommitHashProvider { | |
| return func(signature *types.Signature, header *types.Header, proposerAddress []byte) (types.Hash, error) { | |
| abciCommit := rpc.GetABCICommit(header.Height(), header.Hash(), proposerAddress, header.Time(), *signature) | |
| return types.Hash(abciCommit.Hash()), nil | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/rollkit_adapter/commit_hasher.go between lines 9 and 16, remove the
redundant assignments to abciCommit.Signatures[0].ValidatorAddress and
abciCommit.Signatures[0].Timestamp on lines 12 and 13, since GetABCICommit
already sets these fields with the correct values. Simply delete these two lines
to clean up the code.
pkg/rollkit_adapter/signer.go
Outdated
| package rollkitadapter | ||
|
|
||
| import ( | ||
| cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" | ||
| cmtypes "github.com/cometbft/cometbft/types" | ||
| "github.com/rollkit/rollkit/types" | ||
|
|
||
| "github.com/rollkit/go-execution-abci/pkg/rpc" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix import ordering.
The imports are not properly formatted according to the project's conventions.
Run gci to fix the import ordering, or apply this diff:
package rollkitadapter
import (
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
cmtypes "github.com/cometbft/cometbft/types"
+
"github.com/rollkit/rollkit/types"
"github.com/rollkit/go-execution-abci/pkg/rpc"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package rollkitadapter | |
| import ( | |
| cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" | |
| cmtypes "github.com/cometbft/cometbft/types" | |
| "github.com/rollkit/rollkit/types" | |
| "github.com/rollkit/go-execution-abci/pkg/rpc" | |
| ) | |
| package rollkitadapter | |
| import ( | |
| cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" | |
| cmtypes "github.com/cometbft/cometbft/types" | |
| "github.com/rollkit/rollkit/types" | |
| "github.com/rollkit/go-execution-abci/pkg/rpc" | |
| ) |
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 5-5:
File is not properly formatted (gci)
🪛 GitHub Actions: CI and Release
[error] 5-5: File is not properly formatted (gci)
🤖 Prompt for AI Agents
In pkg/rollkit_adapter/signer.go at the top of the file (lines 1-10), the import
statements are not ordered according to the project's conventions. To fix this,
run the `gci` tool on the file to automatically reorder the imports correctly,
or manually reorder them to group standard library imports, third-party imports,
and local imports separately and alphabetically within each group.
| package rollkitadapter | ||
|
|
||
| import ( | ||
| "bytes" | ||
| stdsha256 "crypto/sha256" | ||
| "encoding/hex" | ||
| "fmt" | ||
|
|
||
| "cosmossdk.io/log" | ||
| tmcryptoed25519 "github.com/cometbft/cometbft/crypto/ed25519" | ||
| tmtypes "github.com/cometbft/cometbft/types" | ||
| "github.com/libp2p/go-libp2p/core/crypto" | ||
| rollkittypes "github.com/rollkit/rollkit/types" | ||
| ) |
There was a problem hiding this comment.
Fix import formatting.
The imports need to be properly formatted according to the project's linting rules.
Apply this diff to fix the import formatting:
package rollkitadapter
import (
"bytes"
stdsha256 "crypto/sha256"
"encoding/hex"
"fmt"
"cosmossdk.io/log"
tmcryptoed25519 "github.com/cometbft/cometbft/crypto/ed25519"
tmtypes "github.com/cometbft/cometbft/types"
"github.com/libp2p/go-libp2p/core/crypto"
+
rollkittypes "github.com/rollkit/rollkit/types"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package rollkitadapter | |
| import ( | |
| "bytes" | |
| stdsha256 "crypto/sha256" | |
| "encoding/hex" | |
| "fmt" | |
| "cosmossdk.io/log" | |
| tmcryptoed25519 "github.com/cometbft/cometbft/crypto/ed25519" | |
| tmtypes "github.com/cometbft/cometbft/types" | |
| "github.com/libp2p/go-libp2p/core/crypto" | |
| rollkittypes "github.com/rollkit/rollkit/types" | |
| ) | |
| package rollkitadapter | |
| import ( | |
| "bytes" | |
| stdsha256 "crypto/sha256" | |
| "encoding/hex" | |
| "fmt" | |
| "cosmossdk.io/log" | |
| tmcryptoed25519 "github.com/cometbft/cometbft/crypto/ed25519" | |
| tmtypes "github.com/cometbft/cometbft/types" | |
| "github.com/libp2p/go-libp2p/core/crypto" | |
| rollkittypes "github.com/rollkit/rollkit/types" | |
| ) |
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 12-12:
File is not properly formatted (gci)
🤖 Prompt for AI Agents
In pkg/rollkit_adapter/validator_hasher.go at lines 1 to 14, the import
statements are not properly formatted according to the project's linting rules.
Reformat the import block to group standard library imports separately from
third-party imports, ensure consistent indentation and spacing, and remove any
unnecessary blank lines between import groups to comply with the project's style
guidelines.
| var commit = &cmttypes.Commit{ | ||
| Height: int64(blockHeight), | ||
| Round: 0, | ||
| Signatures: []cmttypes.CommitSig{ | ||
| { | ||
| BlockIDFlag: cmttypes.BlockIDFlagCommit, | ||
| ValidatorAddress: s.Validators.Proposer.Address, | ||
| Timestamp: time.Now(), | ||
| Signature: []byte{}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if blockHeight > 1 { | ||
| header, data, err := a.RollkitStore.GetBlockData(ctx, blockHeight-1) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("failed to get previous block data: %w", err) | ||
| } | ||
|
|
||
| commit = &cmttypes.Commit{ | ||
| Height: int64(header.Height()), | ||
| Round: 0, | ||
| BlockID: cmttypes.BlockID{Hash: bytes.HexBytes(header.Hash()), PartSetHeader: cmttypes.PartSetHeader{Total: 1, Hash: bytes.HexBytes(data.Hash())}}, | ||
| Signatures: []cmttypes.CommitSig{ | ||
| { | ||
| BlockIDFlag: cmttypes.BlockIDFlagCommit, | ||
| ValidatorAddress: cmttypes.Address(header.ProposerAddress), | ||
| Timestamp: header.Time(), | ||
| Signature: header.Signature, | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant commit construction logic.
The commit construction logic appears twice - once for proposedLastCommit (lines 314-326) and again here (lines 473-485). This duplication could lead to maintenance issues.
Consider extracting the commit construction into a helper function:
+func (a *Adapter) buildCommitFromPreviousBlock(ctx context.Context, blockHeight uint64) (*cmttypes.Commit, error) {
+ if blockHeight <= 1 {
+ return &cmttypes.Commit{
+ Height: int64(blockHeight),
+ Round: 0,
+ Signatures: []cmttypes.CommitSig{
+ {
+ BlockIDFlag: cmttypes.BlockIDFlagCommit,
+ ValidatorAddress: nil, // or appropriate default
+ Timestamp: time.Now(),
+ Signature: []byte{},
+ },
+ },
+ }, nil
+ }
+
+ header, data, err := a.RollkitStore.GetBlockData(ctx, blockHeight-1)
+ if err != nil {
+ return nil, fmt.Errorf("failed to get previous block data: %w", err)
+ }
+
+ return &cmttypes.Commit{
+ Height: int64(header.Height()),
+ Round: 0,
+ BlockID: cmttypes.BlockID{Hash: bytes.HexBytes(header.Hash()), PartSetHeader: cmttypes.PartSetHeader{Total: 1, Hash: bytes.HexBytes(data.Hash())}},
+ Signatures: []cmttypes.CommitSig{
+ {
+ BlockIDFlag: cmttypes.BlockIDFlagCommit,
+ ValidatorAddress: cmttypes.Address(header.ProposerAddress),
+ Timestamp: header.Time(),
+ Signature: header.Signature,
+ },
+ },
+ }, nil
+}Then use it in both places to eliminate duplication.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/adapter/adapter.go around lines 454 to 486, the commit construction logic
is duplicated, which can cause maintenance problems. Extract the commit creation
code into a separate helper function that takes necessary parameters like block
height and block data, then call this helper function in both places where the
commit is constructed to remove redundancy and improve code maintainability.
| var headerHash types.Hash | ||
| if h, ok := metadata[types.HeaderHashKey]; ok { | ||
| headerHash = h.(types.Hash) | ||
| } else { | ||
| a.Logger.Info("header hash not found in metadata, running genesis block") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for the header hash type assertion.
While the code handles the case when header hash is not found (genesis block), the type assertion on line 297 could panic if the metadata contains an unexpected type.
Add type assertion safety check:
var headerHash types.Hash
if h, ok := metadata[types.HeaderHashKey]; ok {
- headerHash = h.(types.Hash)
+ if hash, ok := h.(types.Hash); ok {
+ headerHash = hash
+ } else {
+ return nil, 0, fmt.Errorf("invalid header hash type in metadata: expected types.Hash, got %T", h)
+ }
} else {
a.Logger.Info("header hash not found in metadata, running genesis block")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var headerHash types.Hash | |
| if h, ok := metadata[types.HeaderHashKey]; ok { | |
| headerHash = h.(types.Hash) | |
| } else { | |
| a.Logger.Info("header hash not found in metadata, running genesis block") | |
| } | |
| var headerHash types.Hash | |
| if h, ok := metadata[types.HeaderHashKey]; ok { | |
| if hash, ok := h.(types.Hash); ok { | |
| headerHash = hash | |
| } else { | |
| return nil, 0, fmt.Errorf( | |
| "invalid header hash type in metadata: expected types.Hash, got %T", h, | |
| ) | |
| } | |
| } else { | |
| a.Logger.Info("header hash not found in metadata, running genesis block") | |
| } |
🤖 Prompt for AI Agents
In pkg/adapter/adapter.go around lines 295 to 300, the type assertion for
headerHash from metadata can panic if the type is unexpected. Modify the code to
use the two-value form of type assertion to safely check the type. If the
assertion fails, log an appropriate error or handle the case gracefully instead
of panicking.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| @@ -45,6 +45,7 @@ import ( | |||
| "github.com/rollkit/rollkit/sequencers/single" | |||
|
|
|||
| "github.com/rollkit/go-execution-abci/pkg/adapter" | |||
| rollkit_adapter "github.com/rollkit/go-execution-abci/pkg/rollkit_adapter" | |||
There was a problem hiding this comment.
The import alias rollkit_adapter doesn't match the package name rollkitadapter declared in the source files. For consistency with Go conventions, consider using the same name as the package declaration without underscores. This makes imports more predictable and follows Go's standard practice of using short, lowercase, single-word package names.
| rollkit_adapter "github.com/rollkit/go-execution-abci/pkg/rollkit_adapter" | |
| rollkitadapter "github.com/rollkit/go-execution-abci/pkg/rollkit_adapter" |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| { | ||
| BlockIDFlag: cmttypes.BlockIDFlagCommit, | ||
| ValidatorAddress: s.Validators.Proposer.Address, | ||
| Timestamp: time.Now(), |
There was a problem hiding this comment.
The timestamp for the commit signature should match the block's timestamp rather than using time.Now(). Using the current time creates inconsistency between the block timestamp and commit timestamp, which could cause validation issues. Consider using the block's timestamp value instead to ensure consistency throughout the block data structure.
| Timestamp: time.Now(), | |
| Timestamp: blockTimestamp, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| errMsg := fmt.Sprintf("ValidatorHasher: CRITICAL MISMATCH - derived validator address (%s) does not match expected proposer address (%s). PubKey used for derivation: %s", | ||
| hex.EncodeToString(derivedAddress), | ||
| hex.EncodeToString(proposerAddress), | ||
| hex.EncodeToString(cometBftPubKey.Bytes())) |
There was a problem hiding this comment.
Consider breaking this error message string across multiple lines to improve readability and adhere to line length guidelines. For example:
errMsg := fmt.Sprintf(
"ValidatorHasher: CRITICAL MISMATCH - derived validator address (%s) "+
"does not match expected proposer address (%s). "+
"PubKey used for derivation: %s",
hex.EncodeToString(derivedAddress),
hex.EncodeToString(proposerAddress),
hex.EncodeToString(cometBftPubKey.Bytes()),
)| errMsg := fmt.Sprintf("ValidatorHasher: CRITICAL MISMATCH - derived validator address (%s) does not match expected proposer address (%s). PubKey used for derivation: %s", | |
| hex.EncodeToString(derivedAddress), | |
| hex.EncodeToString(proposerAddress), | |
| hex.EncodeToString(cometBftPubKey.Bytes())) | |
| errMsg := fmt.Sprintf("ValidatorHasher: CRITICAL MISMATCH - derived validator address (%s) does not match expected proposer address (%s). PubKey used for derivation: %s", | |
| hex.EncodeToString(derivedAddress), | |
| hex.EncodeToString(proposerAddress), | |
| hex.EncodeToString(cometBftPubKey.Bytes())) |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| var headerHash types.Hash | ||
| if h, ok := metadata[types.HeaderHashKey]; ok { | ||
| headerHash = h.(types.Hash) | ||
| } else { | ||
| a.Logger.Info("header hash not found in metadata, running genesis block") | ||
| } |
There was a problem hiding this comment.
The code is attempting to access metadata[types.HeaderHashKey] without first checking if metadata is nil. This could cause a panic when metadata is nil (which happens in the test). Consider adding a nil check before accessing the map:
if metadata != nil {
if h, ok := metadata[types.HeaderHashKey]; ok {
headerHash = h.(types.Hash)
} else {
a.Logger.Info("header hash not found in metadata, running genesis block")
}
} else {
a.Logger.Info("metadata is nil, running genesis block")
}This will prevent nil pointer dereference panics when metadata is nil.
| var headerHash types.Hash | |
| if h, ok := metadata[types.HeaderHashKey]; ok { | |
| headerHash = h.(types.Hash) | |
| } else { | |
| a.Logger.Info("header hash not found in metadata, running genesis block") | |
| } | |
| var headerHash types.Hash | |
| if metadata != nil { | |
| if h, ok := metadata[types.HeaderHashKey]; ok { | |
| headerHash = h.(types.Hash) | |
| } else { | |
| a.Logger.Info("header hash not found in metadata, running genesis block") | |
| } | |
| } else { | |
| a.Logger.Info("metadata is nil, running genesis block") | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,16 @@ | |||
| package rollkitadapter | |||
There was a problem hiding this comment.
The package name rollkitadapter doesn't match its directory name rollkit_adapter. Go convention recommends that package names match their directory names, using lowercase single words without underscores. Consider either:
- Renaming the package to
rollkit_adapterto match the directory, or - Renaming the directory to
rollkitadapterto match the package name
This will improve code organization and make imports more intuitive for other developers.
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/rollkit_adapter/header_hasher.go (1)
9-12: Consider avoiding global state modification in init function.The
initfunction automatically sets a global header hasher when the package is imported, which can lead to unexpected side effects and makes testing more difficult. Consider explicit initialization instead.-// init automatically sets the CometBFT header hasher as the default when this package is imported -func init() { - rollkittypes.SetHeaderHasher(CreateCometBFTHeaderHasher()) -}Then require explicit initialization where needed:
// In your main setup code rollkittypes.SetHeaderHasher(rollkitadapter.CreateCometBFTHeaderHasher())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)pkg/rollkit_adapter/header_hasher.go(1 hunks)server/start.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- server/start.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint / golangci-lint
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (2)
pkg/rollkit_adapter/header_hasher.go (2)
14-23: LGTM! Header hasher implementation is well-structured.The function correctly:
- Handles the conversion from Rollkit header to ABCI header
- Provides proper error handling for conversion failures
- Returns the appropriate hash type
- Follows the expected HeaderHasher function signature
1-7: LGTM! Package structure and imports are appropriate.The package name and imports are well-organized and necessary for the CometBFT integration functionality.
Overview
Related to rollkit/rollkit#2301
Summary by CodeRabbit